Skip to content

feat!: replace hybrid CJS/ESM build with ESM-only - #9536

Open
Mrtenz wants to merge 18 commits into
mrtenz/drop-node-20from
mrtenz/esm
Open

feat!: replace hybrid CJS/ESM build with ESM-only#9536
Mrtenz wants to merge 18 commits into
mrtenz/drop-node-20from
mrtenz/esm

Conversation

@Mrtenz

@Mrtenz Mrtenz commented Jul 16, 2026

Copy link
Copy Markdown
Member

Explanation

Currently, packages in this monorepo are built as both CommonJS and ESM using @ts-bridge/cli. This PR replaces that with ESM-only output built with standard tsc and TypeScript project references.

The motivation is to simplify the build toolchain by removing the ts-bridge dependency in preparation for the TypeScript 6+ upgrade.

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

High Risk
Breaking publish surface for every @metamask/* package (ESM-only, no CommonJS entrypoints) plus a repo-wide build and test harness change that all downstream clients must adopt.

Overview
Replaces @ts-bridge/cli with tsc --build across the monorepo so published packages emit ESM-only output (dist/index.js + dist/index.d.ts) instead of dual .cjs/.mjs artifacts. Root and package package.json files add "type": "module", drop main / conditional import/require exports, and swap runtime lodash for lodash-es with matching source import updates.

Tooling follow-through: CI partial builds call yarn tsc --build; build:only-clean also removes tsconfig.build.tsbuildinfo; @ts-bridge/* is removed from Yarn preapproval; building docs no longer mention ts-bridge. Jest is retuned so package tests compile sources to CommonJS via ts-jest (including lodash-eslodash mapping) while script tests stay ESM; shared configs move to .cjs, jest environment files to jest.environment.cjs, and ESLint treats .js as ESM where appropriate. Wallet-cli daemon e2e drops Node 20 from the matrix.

Reviewed by Cursor Bugbot for commit bf31af2. Bugbot is set up for automated code reviews on this repo. Configure here.

@Mrtenz Mrtenz changed the title feat: replace hybrid CJS/ESM build with ESM-only feat!: replace hybrid CJS/ESM build with ESM-only Jul 16, 2026
@Mrtenz
Mrtenz changed the base branch from main to esm July 16, 2026 17:14
@Mrtenz
Mrtenz force-pushed the mrtenz/esm branch 3 times, most recently from 4a98147 to db128a5 Compare July 16, 2026 17:55
@socket-security

socket-security Bot commented Jul 16, 2026

Copy link
Copy Markdown

All alerts resolved. Learn more about Socket for GitHub.

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report

@Mrtenz
Mrtenz force-pushed the mrtenz/esm branch 16 times, most recently from d99b699 to 2438c87 Compare July 21, 2026 09:35
@Mrtenz
Mrtenz force-pushed the mrtenz/esm branch 5 times, most recently from 91b5c8e to fa18841 Compare August 20, 2026 10:18
@cursor
cursor Bot requested review from cryptodev-2s and mcmire August 26, 2026 10:38
@Mrtenz
Mrtenz changed the base branch from esm to mrtenz/drop-node-20 August 26, 2026 13:11
@Gudahtt

Gudahtt commented Aug 26, 2026

Copy link
Copy Markdown
Member

I've assigned the DO-NOT-MERGE label temporarily until we've discussed how to roll this out to clients, and have prepared for it adequately. As this will be a breaking change for all packages, we'll need to be prepared to roll this out quickly to avoid disruption.

@Mrtenz

Mrtenz commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

I've assigned the DO-NOT-MERGE label temporarily until we've discussed how to roll this out to clients, and have prepared for it adequately. As this will be a breaking change for all packages, we'll need to be prepared to roll this out quickly to avoid disruption.

@Gudahtt This PR targets mrtenz/drop-node-20. I've applied the label to that PR instead.

"scripts": {
"build": "ts-bridge --project tsconfig.build.json --verbose --clean --no-references",
"build:all": "ts-bridge --project tsconfig.build.json --verbose --clean",
"build": "tsc --project tsconfig.build.json",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously yarn build would automatically clean dist. With these changes it appears that is no longer the case. Is it worth adding a build:clean script to all packages in case that is needed? Or should we drop it for now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we can add a build:clean script to all packages. If we make it clean by default we don't get the benefits of incremental builds.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that sounds like a good plan.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Comment on lines +464 to +468
// TS2742: The inferred type of 'selectAllAssets' cannot be named without a
// reference to '../../../../node_modules/@metamask/account-tree-controller/dist/wallet.js'.
// This is likely not portable. A type annotation is necessary.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export const selectAllAssets: any = createAssetListSelector(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's definitely interesting...

Comment thread packages/authenticated-user-storage/package.json
Comment thread packages/java-tron-up/package.json
Comment thread packages/keyring-controller/src/KeyringController.ts
Comment thread scripts/create-package/cli.test.ts
Comment thread jest.config.packages.cjs
Comment thread jest.config.scripts.cjs
Comment thread package.json
Comment thread .github/workflows/lint-build-test.yml
Comment on lines 24 to 27
".": {
"import": {
"types": "./dist/index.d.mts",
"default": "./dist/index.mjs"
},
"require": {
"types": "./dist/index.d.cts",
"default": "./dist/index.cjs"
}
"types": "./dist/index.d.ts",
"default": "./dist/index.js"
},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document this in the changelog as a breaking change?

"types": "./dist/index.d.cts",
"default": "./dist/index.cjs"
}
"types": "./dist/index.d.ts",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document this in the changelog?

(and so on for other packages)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6f6a2bb. Configure here.

Comment thread yarn.config.cjs
expectWorkspaceField(
workspace,
'scripts.build:only-clean',
`rimraf './dist' './tsconfig.build.tsbuildinfo'`,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Template omits required clean scripts

Medium Severity

New workspace constraints require build:clean and build:only-clean, but the create-package template still omits those scripts and the rimraf dependency. Packages created with yarn create-package fail yarn constraints immediately.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6f6a2bb. Configure here.

Mrtenz added 18 commits August 27, 2026 23:29
Switch from `@ts-bridge/cli` to standard `tsc` with TypeScript project
references for building all packages.
Remove CJS entry points (`main`, `types`, `require`) from all package
manifests and simplify `exports` to a single `import` condition. This
drops the dual CJS/ESM output in favour of ESM-only distribution.
Mark all packages (and the root) as native ESM by adding
`"type": "module"` to their manifests. Also adds a Yarn constraint
to enforce this field going forward.
`__dirname` is not available in ESM. Replace all usages with
`import.meta.dirname`, which is equivalent and available since
Node.js 21.2.
Older versions of TypeScript break when using named imports in ESM.
Import the default export and destructure instead as a temporary
workaround. Also passes `--formatter oxfmt` to the
`messenger-action-types` scripts in packages missing it.
Adding `"type": "module"` to every package made each `jest.config.js` an
ES module, so the `require` and `module.exports` they are written with no
longer resolved and Jest could not start at all.

Rather than convert the test setup to ESM, the configs move to `.cjs` and
Jest keeps running as CommonJS. A `ts-jest` transform override compiles
the sources to CommonJS for tests only, which leaves the published build
untouched. This works because Jest treats TypeScript as ESM only when the
extension is listed in `extensionsToTreatAsEsm`, which it is not here.

Keeping the tests on CommonJS avoids importing `jest` from
`@jest/globals` in every test file, swapping `lodash` for `lodash-es`,
and rewriting `jest.mock` calls as `jest.unstable_mockModule`. It also
resolves the failures where an external CommonJS dependency could not
`require()` a monorepo package that had become ESM-only.

The trade-off is that the suite exercises CommonJS while the packages
ship ESM, so genuinely ESM-only problems are caught by the build and type
checks rather than by the tests.
`"type": "module"` in the root manifest makes every `.js` file an ES
module, so `.prettierrc.js` could no longer use `module.exports`. It moves
to `.prettierrc.cjs`, and the entry point in `knip.config.mts` is updated
to match.

The `permission-controller` ESLint config is removed rather than renamed.
It extends a root `.eslintrc.js` that no longer exists now that the repo
uses flat config in `eslint.config.mjs`, and nothing references it, so it
has no effect.
The packages compile to CommonJS for tests, but the scripts cannot: they
use `import.meta`, which has no CommonJS equivalent. Their Jest config
opts into ESM instead, through `extensionsToTreatAsEsm` and the `useESM`
option of `ts-jest`.

Running as ESM means `require` and the injected `jest` global are gone, so
`utils.ts` loads the Prettier config through `createRequire`, and the test
files import `jest` from `@jest/globals`. `jest.mock` has no effect on ES
modules, so the mocked modules move to `jest.unstable_mockModule` and are
pulled in with dynamic imports afterwards.

The `process.exit` spy in `cli.test.ts` now has to be installed before
`cli.js` is imported. The ESM build of yargs captures `process.exit` by
reference when it loads, so a spy installed later is never consulted, and
its implementation is re-armed per test because `resetMocks` clears it.
`lodash` is CommonJS, so its named exports cannot be imported from an ES
module: `import { cloneDeep } from 'lodash'` throws at run time once the
packages are loaded as ESM. This went unnoticed because the tests compile
to CommonJS, where the same import is fine. It surfaced in the
`wallet-cli` daemon end-to-end test, which spawns the built CLI and so
loads the real ES modules.

The sources now import `lodash-es`, which publishes the same API as an ES
module. Jest maps it back to `lodash` for the tests, since the reverse
problem applies there: an ES module cannot be `require`d.

Two call sites needed more than a rename. `smart-transactions-controller`
imported the default export, which `lodash-es` does not provide, so it
imports the functions it uses by name. `assets-controller` referred to the
`MemoizedFunction` type, which `@types/lodash-es` does not re-export, so
it takes that type from `@types/lodash` instead; the import is erased at
compile time and adds no run-time dependency.
`ethereumjs-wallet` is CommonJS, and Node cannot reliably detect its named
exports, so `import { thirdparty }` throws once the package is loaded as an
ES module. Node 22 fails on it where Node 24 happens to cope, which is why
this only appeared in the `wallet-cli` daemon end-to-end test.

The package also declares a TypeScript-style default export, so
`module.exports` is reached through `default` under Node's interop but is
the namespace itself once `esModuleInterop` has unwrapped it for the
CommonJS test build. Both shapes are now resolved explicitly, which also
fixes `Wallet.fromV3` being `undefined` under ESM.
Support for Node 18 and 20 was dropped in #9168, which set `engines` to
`^22.14.0 || ^24` and trimmed the test matrices, but the `wallet-cli`
daemon end-to-end job kept its own matrix and was missed. Node 20 also
lacks the ESM interop these packages now rely on.
The build info files record which outputs `tsc` has already produced, so
deleting `dist` without them leaves the two disagreeing. Incremental builds
then skip work whose output is gone, which surfaces as a cascade of TS6305
"has not been built from source file" errors that a clean build does not
resolve.
Under `moduleResolution: "Node16"`, TypeScript decides whether a
declaration file is ESM or CommonJS from the nearest `package.json`
`"type"` field. `@metamask/safe-event-emitter` declares no `"type"`, so
its ESM declarations were read as CommonJS and the default import
resolved to the module namespace rather than the class, leaving
`class JsonRpcEngine extends SafeEventEmitter` failing to build.

That was carried as a patch adding an `index.d.mts`, which TypeScript
always treats as ESM. Version 3.1.3 ships the same fix, so the patch and
its resolutions are removed in favour of the released version. Pinning to
a patched `3.1.1` would also have silently dropped the fix as soon as
anything asked for a newer version.
…-controller`

`lodash-es` is already a dependency of this package for other uses, so
`klona` was an unnecessary second deep-clone implementation. Using
`cloneDeep` avoids the extra dependency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants